Skip to content

fix(state): drain, cap and time-bound a state hook's output - #11

Merged
aeddi merged 11 commits into
mainfrom
state-hook-drains-stdout
Nov 4, 2026
Merged

aeddi merged 11 commits into
mainfrom
state-hook-drains-stdout

Conversation

@aeddi

@aeddi aeddi commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

A state hook's output is now read while the hook runs, capped at 1 MiB, and bounded by the
hook's own timeout. Three problems are fixed:

  • A hook writing more than a pipe holds deadlocked. tmkms waited for the hook to exit
    before reading its stdout, so a hook that filled the pipe (64 KiB on Linux with 4 KiB pages,
    and on macOS) blocked on the write, was killed at the timeout, and was reported as
    "timed out" instead of read.
  • Reading had no size limit. Draining while the hook runs would otherwise let a runaway
    hook grow the signer's memory without bound. The reader stops one byte past 1 MiB, and an
    oversized hook is reported as such. The limit is checked before the exit status, because a
    hook cut off at the limit dies of SIGPIPE and its status alone would not say why.
  • A process the hook left behind could hold startup open. Reading waited for every holder
    of stdout to close it, however long that took. It now shares the hook's deadline.

timeout_secs's doc now gives its real default, 1 second rather than 5, and says it bounds
closing stdout too.

Safety

The state hook supplies the starting height for double-sign protection. Every hook error,
including the two new ones (output over the limit, output held open past the deadline), goes
through the same fail_closed policy: with true the signer refuses to start, with false
it logs the error and starts from the state file.

One behaviour does change. A process holding the hook's stdout open used to block startup
indefinitely, so neither policy ever applied; it now fails at the deadline and takes the
policy like any other error.

Double-sign protection is never weakened: an error never touches the state, and a successful
read is still parsed by serde_json, which rejects trailing data, and checked against the
same sanity limit.

Changes

  • Drain the output while the hook runs — d0228d4
  • Start the reader with thread::Builder, so failing to start it is an error, not a panic — 8b15ed1
  • Bound reading the output by the hook's timeout — 7d4fc3d
  • Cap the output at 1 MiB, and pin the cap with a test — c5b4201, a4a9ef7
  • Give timeout_secs's real default — a91971c
  • Keep hook tests from spawning processes at the same time — 2168377

Testing

  • Each behaviour fix is test-first, the test failing before the fix: a writer past the pipe
    buffer was reported as a timeout; a 2 MiB writer was accepted; echo json; sleep 5 & with
    a 1 s timeout returned Ok after 5.19 s.
  • Mutants: removing the cap fails reading_stops_one_byte_past_the_limit; checking the limit
    after the exit status fails output_past_the_limit_is_an_error with "status None";
    draining only after the hook exits, and an unbounded receive, are each killed too.
  • The hook tests now take turns spawning processes. On macOS, std creates a pipe and marks it
    close-on-exec in two steps, so a child spawned on another test thread could inherit a test's
    pipe and hold its output open. Production runs on Linux, where pipe2(O_CLOEXEC) is
    atomic, and spawns hooks one at a time. Stress runs: about 1 failure in 30 before, none in
    140 after.
  • 68 unit + 23 integration tests pass. cargo clippy --all-features, the command CI runs,
    is clean with -Dwarnings, and so is this file's test module (--lib --profile test).

@aeddi
aeddi force-pushed the state-hook-drains-stdout branch from 317f131 to ef380d7 Compare November 4, 2026 11:30
@aeddi
aeddi marked this pull request as ready for review November 4, 2026 11:33
@aeddi
aeddi merged commit 6185253 into main Nov 4, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant